-
-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
delegate to Node on non-Mocha unhandled rejections #4489
Conversation
This is not intended as a _fix_ for #4481, since it's possible that Mocha's behavior in v8.2.0 uncovers false positives. In other cases--depending on the use-case--they are not false positives at all, but rather annoyances that depended on the pre-v15 behavior of Node.js to only issue warnings. This PR changes the behavior of Mocha so that it will re-emit unhandled rejections to `process` _if_ they did not generate from Mocha. If the rejection generated from Mocha, then we treat it as an uncaught exception, because Mocha should not be in the business of ignoring its own unhandled rejections. The logic for detecting an "error originating from Mocha" is not exhaustive. Once an unhandled rejection is re-emitted to `process`, Node decides what to do with it based on how it is configured to handle unhandled rejections (strict, warn, quiet, etc.). Added a public method to `errors` module; `isMochaError()` Ref: #4481
think I left some test changes out of the commit, will fix it tomorrow. |
I don't think this code is safe anyway (see weird "multiple done" issue cropping up in #4481). It's pretty clear the changes I made in v8.2.0 screwed something up w/r/t handling of promises, but I'm not sure what that is yet. |
abc4855
to
2b38221
Compare
Signed-off-by: Christopher Hiller <boneskull@boneskull.com>
2b38221
to
fc0b6cd
Compare
this actually addresses issues in #4481 and another issue causing erroneous "multiple done" output. |
This update causes a LOT of test failures for us in Heroku. We had to revert back to 8.2.0. |
unless you can provide more info, I can’t help |
@boneskull In my case, it seems to cause an extra listener call. I can repro consistently using mocha 8.2.1. Here's a minimal repro case with no other dependencies. I'm on node 10.19.0. it('should call a custom `unhandledRejection` listener once per unhandled rejection', async () => {
// Register our listener
let listenerCallCount = 0
process.on('unhandledRejection', () => listenerCallCount++)
// Force a rejection and wait for it to bubble up to the process EventEmitter
const unhandledRejection = new Promise(resolve => process.once('unhandledRejection', () => resolve()))
Promise.reject(new Error('Unhandled promise rejection!'))
await unhandledRejection
if (listenerCallCount !== 1) {
throw new Error(`listenerCallCount was ${listenerCallCount}, expected 1`)
}
}) Result:
If I go into So on 8.2.1 we get an extra listener call. On 8.2.0 the test fails with |
yeah, Mocha now has a listener for would recommend that you test for a number greater than 0, or test that a specific listener was called. |
Thanks for the quick reply. I'm a little confused by what you mean by "swallow rejections", since commenting out the re-emit code shows that rejections still reach the user's listener. Maybe there are other listeners it's doesn't reach that I'm not aware of though. In any case, the workarounds you provided are fine for me. Thanks for your work on mocha! |
I mean that users who aren’t setting their own will not see anything and have no idea the rejection happened (depending on node version) |
This is not intended as a fix for #4481, since it's possible that Mocha's behavior in v8.2.0 uncovers false positives. In other cases--depending on the use-case--they are not false positives at all, but rather annoyances that depended on the pre-v15 behavior of Node.js to only issue warnings.
This PR changes the behavior of Mocha so that it will re-emit unhandled rejections to
process
if they did not generate from Mocha. If the rejection generated from Mocha, then we treat it as an uncaught exception, because Mocha should not be in the business of ignoring its own unhandled rejections. The logic for detecting an "error originating from Mocha" is not exhaustive.Once an unhandled rejection is re-emitted to
process
, Node decides what to do with it based on how it is configured to handle unhandled rejections (strict, warn, quiet, etc.).Added a public method to
errors
module;isMochaError()
Ref: #4481